Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds new flag --metrics-host #811

Merged
merged 4 commits into from
Aug 8, 2023
Merged

Conversation

nkinkade
Copy link
Contributor

@nkinkade nkinkade commented Aug 3, 2023

Kured runs with hostNetwork=true, which means that, today, Kured will expose the metrics endpoint on what is probably a public interface for many deployments. The metrics are generally not sensitive data, but as a policy it seems wrong to do this. Allowing the user to configure which port Kured exposes metrics on is a step in the right direction, but the current implementation does not allow the user to specify the address to listen on. For example, in our use case, we would like for the metrics server to only listen on a loopback address. This would allow us to put kube-rbac-proxy in front of the metrics endpoint.

This change could slightly complicate the Helm chart because it not only affects the metrics endpoint, but also the readinessProbe configuration.

@nkinkade
Copy link
Contributor Author

nkinkade commented Aug 3, 2023

I just noticed that a PR was merged yesterday which set hostNetwork=false by default in the chart. That change may obviate the need for this PR, at least for our use case. However, in cases where someone does for some reason want to run with hostNetwork=true it may still be useful for them to be able to specify the full listen address. There may be other reasons I'm not thinking of.

Copy link
Member

@ckotzbauer ckotzbauer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @nkinkade,
thanks for your PR. Yes, since a few days, the hostNetwork=true setting is not the default anymore, so this problem should not occur too often.

But, you are right, it may be good in some cases to also configure the host of the interface to listen on. However, the --metrics-port flag was released with 1.13.2, so this PR would be a breaking change, which we want to avoid. Instead of replacing the flag it would be good to add an additional --metrics-host flag to kured (and also to the chart).

@nkinkade nkinkade changed the title Replaces flag --metrics-port with --metrics-addresss Adds new flag --metrics-host Aug 4, 2023
@nkinkade
Copy link
Contributor Author

nkinkade commented Aug 4, 2023

@ckotzbauer: I have modified the PR and its title. The changes now only add a new flag --metrics-host.

I am happy to contribute a corresponding PR to the charts repo, once I have confirmation from you that this change looks okay.

Thank you.

@ckotzbauer
Copy link
Member

The code looks good, thanks!
Can you please

  • signoff all commits to our repos?
  • open a PR against the chart-repository
  • open a PR against the website-repository and document the flag

Thanks!!

nkinkade added a commit to nkinkade/kured-charts that referenced this pull request Aug 7, 2023
nkinkade added a commit to nkinkade/kured-charts that referenced this pull request Aug 7, 2023
nkinkade added a commit to nkinkade/kured-website that referenced this pull request Aug 7, 2023
The flag --metrics-port already exists. While not as clean, to avoid
introducing a backward incompatible change to flags, this commit adds a
new --metrics-host flag, which in combination with the existing
--metrics-port flag can define a complete listen address for the metrics
server as "<metrics-host>:<metrics-port>"

Signed-off-by: Nathan Kinkade <[email protected]>
@nkinkade
Copy link
Contributor Author

nkinkade commented Aug 7, 2023

@ckotzbauer:

Let me know if I missed anything.

Thank you!

@ckotzbauer ckotzbauer added this to the 1.14.0 milestone Aug 8, 2023
@ckotzbauer
Copy link
Member

Thanks for the changes!!

@ckotzbauer ckotzbauer merged commit 351ca71 into kubereboot:main Aug 8, 2023
11 checks passed
ckotzbauer pushed a commit to kubereboot/website that referenced this pull request Aug 19, 2023
ckotzbauer added a commit to kubereboot/charts that referenced this pull request Aug 19, 2023
kubereboot/kured#811

Signed-off-by: Nathan Kinkade <[email protected]>
Signed-off-by: Christian Kotzbauer <[email protected]>
Co-authored-by: Christian Kotzbauer <[email protected]>
@ckotzbauer ckotzbauer mentioned this pull request Aug 19, 2023
ckotzbauer added a commit to kubereboot/website that referenced this pull request Aug 21, 2023
* doc: add drain-pod-selector (#71)

Signed-off-by: Christian Kotzbauer <[email protected]>

* doc: Documents new --metrics-host flag (#69)

kubereboot/kured#811

Signed-off-by: Nathan Kinkade <[email protected]>

* doc: add version range

Signed-off-by: Christian Kotzbauer <[email protected]>

* doc: add new arguments

Signed-off-by: Christian Kotzbauer <[email protected]>

---------

Signed-off-by: Christian Kotzbauer <[email protected]>
Signed-off-by: Nathan Kinkade <[email protected]>
Co-authored-by: nkinkade <[email protected]>
ckotzbauer added a commit to kubereboot/charts that referenced this pull request Aug 21, 2023
* expose --drain-pod-selector (#43)

Signed-off-by: Boris Pruessmann <[email protected]>
Signed-off-by: Christian Kotzbauer <[email protected]>
Co-authored-by: Christian Kotzbauer <[email protected]>

* Adds new flag --metrics-host to chart (#50)

kubereboot/kured#811

Signed-off-by: Nathan Kinkade <[email protected]>
Signed-off-by: Christian Kotzbauer <[email protected]>
Co-authored-by: Christian Kotzbauer <[email protected]>

* feat: changes for 1.14.0

Signed-off-by: Christian Kotzbauer <[email protected]>

---------

Signed-off-by: Boris Pruessmann <[email protected]>
Signed-off-by: Christian Kotzbauer <[email protected]>
Signed-off-by: Nathan Kinkade <[email protected]>
Co-authored-by: Boris Prüßmann <[email protected]>
Co-authored-by: nkinkade <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants